Minor enhancement : Use Arrow Link for the Force Dependency graph #373
Conversation
Codecov Report
@@ Coverage Diff @@
## master #373 +/- ##
==========================================
- Coverage 89.07% 88.93% -0.15%
==========================================
Files 162 163 +1
Lines 3663 3670 +7
Branches 833 834 +1
==========================================
+ Hits 3263 3264 +1
- Misses 364 369 +5
- Partials 36 37 +1
Continue to review full report at Codecov.
|
|
@tiffon , here is the example for a simple dependency graph : Currently :
Proposal : |
There was a problem hiding this comment.
Right now, the arrows get covered by nodes with a large radius:
We have some nodes that cover the arrow completely.
Can you see if it's possible to account for this? I think there is a targetRadius prop that might do the trick?
|
@tiffon, thank you for the remark. I made some tests and the targetRadius is more for an homothetic ratio which is not great for "big nodes" so I try a translation of the marker on the link and I have something interesting . Unfortunately, I have to patch the ForceGraphArrowLink (https://github.com/uber/react-vis-force/blob/master/src/components/ForceGraphArrowLink.js) . 2 possible solutions :
What do you prefer ? |
|
I see what you mean. It's very unfortunate, but yeah, we're probably not going to get much traction on updating the library, and I would generally prefer not to have a fork of it... @yurishkuro, can you weigh in on supporting arrows in the links? The main issue is that in cases where the nodes have a large radius, the node covers the arrows.
Note: The purple arrows are my annotations. In the image above, the opacity is reduced. This allows the arrows, which are behind the green nodes, to be visible. The dark, concentric effects in the large nodes are the visual effect of the arrows that are behind the nodes. What do you think? |
|
@tiffon , for helping you in the decision, here is the solution with the "monkey_patch" : |
|
@Etienne-Carriere Using a local arrow link works great! In the layout of the edges, I noticed that sometimes the edges are off-center and / or the arrow is hidden behind the target node.
The following changes seem to address these issues: diff --git a/packages/jaeger-ui/src/components/DependencyGraph/DependencyForceGraph.js b/packages/jaeger-ui/src/components/DependencyGraph/DependencyForceGraph.js
index e27ab40..a7e856b 100644
--- a/packages/jaeger-ui/src/components/DependencyGraph/DependencyForceGraph.js
+++ b/packages/jaeger-ui/src/components/DependencyGraph/DependencyForceGraph.js
@@ -60,6 +60,7 @@ export default class DependencyForceGraph extends Component {
render() {
const { nodes, links } = this.props;
const { width, height } = this.state;
+ const nodesMap = new Map(nodes.map(node => [node.id, node]));
return (
<div
@@ -103,7 +104,12 @@ export default class DependencyForceGraph extends Component {
/>
))}
{links.map(({ opacity, ...link }) => (
- <JaegerForceGraphArrowLink key={`${link.source}=>${link.target}`} opacity={opacity} link={link} />
+ <JaegerForceGraphArrowLink
+ key={`${link.source}=>${link.target}`}
+ opacity={opacity}
+ link={link}
+ targetRadius={nodesMap.get(link.target).radius}
+ />
))}
</InteractiveForceGraph>
</div>
diff --git a/packages/jaeger-ui/src/components/DependencyGraph/JaegerForceGraphArrowLink.js b/packages/jaeger-ui/src/components/DependencyGraph/JaegerForceGraphArrowLink.js
index 3160f56..aaf9516 100644
--- a/packages/jaeger-ui/src/components/DependencyGraph/JaegerForceGraphArrowLink.js
+++ b/packages/jaeger-ui/src/components/DependencyGraph/JaegerForceGraphArrowLink.js
@@ -28,7 +28,7 @@ import { ForceGraphLink} from 'react-vis-force';
}
-export default class JaegerForceGraphArrowLink extends PureComponent {
+export default class JaegerForceGraphArrowLink extends PureComponent {
static get propTypes() {
return {
link: PropTypes.shape({
@@ -57,30 +57,25 @@ export default class JaegerForceGraphArrowLink extends PureComponent {
}
render() {
- const { link, targetRadius, edgeOffset, ...spreadable } = this.props;
+ const { link, targetRadius, edgeOffset: _, ...spreadable } = this.props;
const id = `arrow-${linkId(link)}`;
return (
<g>
<defs>
<marker
id={id}
- markerWidth={(targetRadius * 3) + 1}
- markerHeight={(targetRadius * 3) + 1}
- refX={ targetRadius + link.target_node_size }
- refY={targetRadius}
+ markerWidth={6}
+ markerHeight={4}
+ refX={5 + targetRadius}
+ refY={2}
orient="auto"
markerUnits="strokeWidth"
>
- {targetRadius > 0 && (
- <path
- d={`M0,0 L0,${targetRadius * 2} L${targetRadius * 3},${targetRadius} z`}
- fill={spreadable.stroke || spreadable.color}
- />
- )}
+ {targetRadius > 0 && <path d="M0,0 L0,4 L6,2 z" fill={spreadable.stroke || spreadable.color} />}
</marker>
</defs>
- <ForceGraphLink {...spreadable} link={link} edgeOffset={targetRadius} markerEnd={`url(#${id})`} />
+ <ForceGraphLink {...spreadable} link={link} markerEnd={`url(#${id})`} />
</g>
);
}
These changes
What do you think? Apologies for the slow reply! Thank you for the diff! |
ghost
assigned
|
@tiffon , I put the monkey_patch branch and then take in account your patch proposal. After you review, I propose to squash the commits before merge. |
There was a problem hiding this comment.
Thanks, this looks great!
I left a few comments, LMK if anything looks awry.
Also, it turns out the build failed due to the yarn prettier formatting task not being run on the changes. In the future, can you run yarn prettier before pushing?
| @@ -0,0 +1,88 @@ | |||
| // Copyright (c) 2017 Uber Technologies, Inc. | |||
There was a problem hiding this comment.
A couple of requests for this file.
Can you update the 2017 to 2019 in the copyright?
We're aiming to use TypeScript for all new files. Would you mind converting to *.tsx?
Also, I am thinking the Jaeger scoping of JaegerForceGraphArrowLink is kind of implied; what do you think of renaming to just ForceGraphArrowLink?
| stroke: PropTypes.string, | ||
| strokeWidth: PropTypes.number | ||
| }; | ||
| } |
There was a problem hiding this comment.
As we're using TypeScript, can you use a type instead of prop-types? E.g.
type TProps = {
link: {
source: string;
target: string;
value: string;
};
targetRadius: number;
// etc
};
export default class JaegerForceGraphArrowLink extends PureComponent<TProps> {
// ...| import { window } from 'global'; | ||
| import { debounce } from 'lodash'; | ||
| import {default as JaegerForceGraphArrowLink} from './JaegerForceGraphArrowLink'; |
There was a problem hiding this comment.
The { default as ... } is not necessary; it can simply be:
import AnyName from './the-file';
// same as:
import { default as AnyName } from './the-file';| id={id} | ||
| markerWidth={6} | ||
| markerHeight={4} | ||
| refX={ 5 + link.target_node_size } |
There was a problem hiding this comment.
I think you can use targetRadius instead of link.target_node_size? The reason I figured using targetRadius is better is because link.target_node_size was not accurate. But, if your update fixes it, then we can get rid of targetRadius.
|
@tiffon , I take in account all your comments but the conversion from JS to TypeScript . Sorry but I an not an JS dev only a "patcher" |
Codecov Report
@@ Coverage Diff @@
## master #373 +/- ##
==========================================
- Coverage 89.07% 88.93% -0.15%
==========================================
Files 162 163 +1
Lines 3663 3670 +7
Branches 833 834 +1
==========================================
+ Hits 3263 3264 +1
- Misses 364 369 +5
- Partials 36 37 +1
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #373 +/- ##
=========================================
Coverage ? 88.99%
=========================================
Files ? 163
Lines ? 3670
Branches ? 834
=========================================
Hits ? 3266
Misses ? 367
Partials ? 37
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
@Etienne-Carriere Thanks for making the changes! No worries about the TypeScript, I went ahead and converted the new file.
|
ping |
|
pong . Yes, I am late with writing the unit test . Will try this week |
|
The ts linter has some issues as it seems to be the first test in TypeScript. I will try to fix it. |
|
Thanks for adding tests! I think the CI failure is from adding types for jasmine and enzyme. These added types for node, which defines a We generally write tests in plain JavaScript, and it's preferable to be consistent. Can you change the tests to be JS? Or, do you mind if I make the edit? |
Replace simple link by Arrow link in the Force Dependency graph so that the graph is more understandable Signed-off-by: Etienne Carriere <etienne.a.carriere@socgen.com>
Signed-off-by: Etienne Carriere <etienne.a.carriere@socgen.com>
Signed-off-by: Etienne Carriere <etienne.a.carriere@socgen.com>
Signed-off-by: Etienne Carriere <etienne.a.carriere@socgen.com>
Signed-off-by: Joe Farro <joef@uber.com>
Signed-off-by: Etienne Carriere <etienne.a.carriere@socgen.com>
|
I put the test back in JS |
|
@tiffon , do you know why the codecov hook is hanged ? |
|
I restarted the build |
…egertracing#373) Minor enhancement : Use Arrow Link for the Force Dependency graph Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
Replace simple link by Arrow link in the Force Dependency graph so that the graph is more
understandable
Signed-off-by: Etienne Carriere etienne.a.carriere@socgen.com
Which problem is this PR solving?
Currently, in the Force Directed Graph, we don't know the direction of the dependency between 2 services.
Short description of the changes
Replacing "not arrow link" by "Arrow link" in the graph